gh-138577: Fix keyboard shortcuts in getpass with echo_char by CuriousLearner · Pull Request #141597 · python/cpython

CuriousLearner · 2025-11-15T18:07:47Z

When using getpass.getpass(echo_char='*'), keyboard shortcuts like Ctrl+U (kill line), Ctrl+W (erase word), and Ctrl+V (literal next) now work correctly by reading the terminal's control character settings and processing them in non-canonical mode.

Tested with:

import getpass

# Test Ctrl+U (clear line)
# Type "wrong", press Ctrl+U, type "correct"
password = getpass.getpass(echo_char="*")
print(f"Result: {password!r}")
# Expected: 'correct'

# Test Ctrl+W (erase word)
# Type "hello world", press Ctrl+W, type "python"
password = getpass.getpass(echo_char="*")
print(f"Result: {password!r}")
# Expected: 'hello python'

# Test Ctrl+V (literal next)
# Type "test", press Ctrl+V, press Ctrl+U, type "more"
password = getpass.getpass(echo_char="*")
print(f"Result: {password!r}")
# Expected: 'test\x15more' (contains literal Ctrl+U)

Issue: getpass's echo_char should not affect keyboard shortcuts #138577

📚 Documentation preview 📚: https://cpython-previews--141597.org.readthedocs.build/

When using getpass.getpass(echo_char='*'), keyboard shortcuts like Ctrl+U (kill line), Ctrl+W (erase word), and Ctrl+V (literal next) now work correctly by reading the terminal's control character settings and processing them in non-canonical mode.

picnixz

Thanks for doing this but this looks like a very complicate patch which is what I feared :( Also, I personally would use Ctrl+a/Ctrl+e for jumping at the start/end of what I write and ctrl+k (I think people are more familiar with ctrl+a/ctrl+k rather than ctrl+u for erasing an entire line in general...).

So I'm not really confident in actually changing this. I think deleting the last character that was typed (with DEL) is usually what people expect.

In addition, to make the feature more extendable in the future, I would prefer a tokenizer-based + dispatcher based approach instead (like we currently do for the REPL) because with many ifs like that it becomes hard to follow what's being done. But this would require an refactoring of this module which I don't really know if it's worth.

picnixz · 2025-11-15T18:16:21Z

Lib/getpass.py

+        # Control chars from termios are bytes, convert to str
+        erase_char = term_ctrl_chars['ERASE'].decode('latin-1') if isinstance(term_ctrl_chars['ERASE'], bytes) else term_ctrl_chars['ERASE']
+        kill_char = term_ctrl_chars['KILL'].decode('latin-1') if isinstance(term_ctrl_chars['KILL'], bytes) else term_ctrl_chars['KILL']
+        werase_char = term_ctrl_chars['WERASE'].decode('latin-1') if isinstance(term_ctrl_chars['WERASE'], bytes) else term_ctrl_chars['WERASE']
+        lnext_char = term_ctrl_chars['LNEXT'].decode('latin-1') if isinstance(term_ctrl_chars['LNEXT'], bytes) else term_ctrl_chars['LNEXT']


Those are too long loines and they are unreadable. All of that can be a single function that is given the action to perform and the current capabilities.

picnixz · 2025-11-15T18:16:46Z

Lib/getpass.py

+        erase_char = '\x7f'  # DEL
+        kill_char = '\x15'   # Ctrl+U
+        werase_char = '\x17' # Ctrl+W
+        lnext_char = '\x16'  # Ctrl+V


Ideally, we should have them defined in a global private dict instead to ease maintenance.

picnixz · 2025-11-15T18:17:17Z

Lib/getpass.py

+                    term_ctrl_chars = {
+                        'ERASE': cc[termios.VERASE] if termios.VERASE < len(cc) else b'\x7f',
+                        'KILL': cc[termios.VKILL] if termios.VKILL < len(cc) else b'\x15',
+                        'WERASE': cc[termios.VWERASE] if termios.VWERASE < len(cc) else b'\x17',
+                        'LNEXT': cc[termios.VLNEXT] if termios.VLNEXT < len(cc) else b'\x16',
+                    }


This code must be refactored with a combination of a global dict with the defaults and a function.

bedevere-app · 2025-11-15T18:17:26Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

CuriousLearner · 2025-11-15T21:31:19Z

Also, I personally would use Ctrl+a/Ctrl+e for jumping at the start/end of what I write and ctrl+k (I think people are more familiar with ctrl+a/ctrl+k rather than ctrl+u for erasing an entire line in general...).

Agree on this, since the original ticket asked for Ctrl+U, I would handle all these Ctrl characters.

In addition, to make the feature more extendable in the future, I would prefer a tokenizer-based + dispatcher based approach instead (like we currently do for the REPL) because with many ifs like that it becomes hard to follow what's being done

I'm refactoring the patch to be more extensible in the future, especially as more control characters are added.

CuriousLearner · 2025-11-16T03:36:22Z

It actually grew a lot bigger than I initially expected, but based on your review, I can iterate over this @picnixz

picnixz · 2025-11-16T09:03:08Z

It looks great. The patch itself is small but the tests are long so I think it is fine. Nonetheless I think it would be better to have it in 3.15 rather than in 3.14.1. WDYT? (i need to review it but not today)

Changes were made.

picnixz

Quick review.

picnixz · 2025-11-16T10:34:09Z

Lib/getpass.py

+        # Handle literal next mode FIRST (Ctrl+V quotes next char)
+        if editor.literal_next:
+            editor.handle_literal_next(char)
+            continue


To reduce the number of continue, just use if-elif constructions here.

picnixz · 2025-11-16T10:35:25Z

Lib/getpass.py

+        handler = dispatch.get(char)
+        if handler:
+            handler()
        else:
-            passwd += char
-            stream.write(echo_char)
-            stream.flush()
-            eof_pressed = False
-    return passwd
+            editor.insert_char(char)
+            editor.eof_pressed = False


Here's a suggestion: don't build a dispatch table via a function but make the editor handles it internally:

editor.handle(char)

picnixz · 2025-11-16T10:42:29Z

Lib/getpass.py

+                     for name, value in ctrl_chars.items()}
+
+    def refresh_display(self):
+        """Redraw the entire password line with asterisks."""


Suggested change

"""Redraw the entire password line with asterisks."""

"""Redraw the entire password line with *echo_char*."""

picnixz · 2025-11-16T10:45:42Z

Lib/getpass.py

+    try:
+        old = termios.tcgetattr(fd)
+        cc = old[6]  # Index 6 is the control characters array
+        return {
+            'ERASE': cc[termios.VERASE] if termios.VERASE < len(cc) else _POSIX_CTRL_CHARS['ERASE'],
+            'KILL': cc[termios.VKILL] if termios.VKILL < len(cc) else _POSIX_CTRL_CHARS['KILL'],
+            'WERASE': cc[termios.VWERASE] if termios.VWERASE < len(cc) else _POSIX_CTRL_CHARS['WERASE'],
+            'LNEXT': cc[termios.VLNEXT] if termios.VLNEXT < len(cc) else _POSIX_CTRL_CHARS['LNEXT'],
+            'EOF': cc[termios.VEOF] if termios.VEOF < len(cc) else _POSIX_CTRL_CHARS['EOF'],
+            'INTR': cc[termios.VINTR] if termios.VINTR < len(cc) else _POSIX_CTRL_CHARS['INTR'],
+            # Ctrl+A/E/K are not in termios, use POSIX defaults
+            'SOH': _POSIX_CTRL_CHARS['SOH'],
+            'ENQ': _POSIX_CTRL_CHARS['ENQ'],
+            'VT': _POSIX_CTRL_CHARS['VT'],
+        }
+    except (termios.error, OSError):
+        return _POSIX_CTRL_CHARS.copy()


Suggested change

try:

old = termios.tcgetattr(fd)

cc = old[6] # Index 6 is the control characters array

return {

'ERASE': cc[termios.VERASE] if termios.VERASE < len(cc) else _POSIX_CTRL_CHARS['ERASE'],

'KILL': cc[termios.VKILL] if termios.VKILL < len(cc) else _POSIX_CTRL_CHARS['KILL'],

'WERASE': cc[termios.VWERASE] if termios.VWERASE < len(cc) else _POSIX_CTRL_CHARS['WERASE'],

'LNEXT': cc[termios.VLNEXT] if termios.VLNEXT < len(cc) else _POSIX_CTRL_CHARS['LNEXT'],

'EOF': cc[termios.VEOF] if termios.VEOF < len(cc) else _POSIX_CTRL_CHARS['EOF'],

'INTR': cc[termios.VINTR] if termios.VINTR < len(cc) else _POSIX_CTRL_CHARS['INTR'],

# Ctrl+A/E/K are not in termios, use POSIX defaults

'SOH': _POSIX_CTRL_CHARS['SOH'],

'ENQ': _POSIX_CTRL_CHARS['ENQ'],

'VT': _POSIX_CTRL_CHARS['VT'],

}

except (termios.error, OSError):

return _POSIX_CTRL_CHARS.copy()

res = _POSIX_CTRL_CHARS.copy()

try:

old = termios.tcgetattr(fd)

cc = old[6] # Index 6 is the control characters array

except (termios.error, OSError):

return res

# Ctrl+A/E/K are not in termios, use POSIX defaults

for name in ["ERASE", "KILL", "WERASE", "LNEXT", "EOF", "INTR"]

cap = getattr(termios, f"V{name}")

if cap < len(cc):

res[name] = cc[flag]

return res

Thanks for the suggestion, there was some minor issue here, which I've fixed in my refactoring

picnixz · 2025-11-16T10:47:28Z

Lib/getpass.py

+
+    def refresh_display(self):
+        """Redraw the entire password line with asterisks."""
+        self.stream.write('\r' + ' ' * (len(self.passwd) + 20) + '\r')


Why are we adding 20 extra characters?

The +20 was an arbitrary buffer and has been removed. The current implementation now uses just len(self.passwd) to clear only the necessary characters:

self.stream.write('\r' + ' ' * len(self.passwd) + '\r')

picnixz · 2025-11-16T10:48:21Z

Lib/getpass.py

+        self.ctrl = {name: _decode_ctrl_char(value)
+                     for name, value in ctrl_chars.items()}


Can't we have the POSIX defaults already decoded?

Refactored this in b8609bd

picnixz · 2025-11-16T10:48:58Z

Lib/getpass.py

+            'WERASE': cc[termios.VWERASE] if termios.VWERASE < len(cc) else _POSIX_CTRL_CHARS['WERASE'],
+            'LNEXT': cc[termios.VLNEXT] if termios.VLNEXT < len(cc) else _POSIX_CTRL_CHARS['LNEXT'],
+            'EOF': cc[termios.VEOF] if termios.VEOF < len(cc) else _POSIX_CTRL_CHARS['EOF'],
+            'INTR': cc[termios.VINTR] if termios.VINTR < len(cc) else _POSIX_CTRL_CHARS['INTR'],


I'm assuming cc[termios.VINTR] gives bytes right? in this case we can already decode it.

Yes, cc[termios.VINTR] returns bytes. This is now decoded inline in the refactored implementation:

for name in ('ERASE', 'KILL', 'WERASE', 'LNEXT', 'EOF', 'INTR'): cap = getattr(termios, f'V{name}') if cap < len(cc): res[name] = cc[cap].decode('latin-1')

The _POSIX_CTRL_CHARS defaults are also now stored as already-decoded strings.

CuriousLearner · 2025-11-30T18:42:04Z

Hi @picnixz 👋🏼

Sorry, it took some time to address the review here. I've refactored the patch & I think Python 3.15 would be a better choice for this, though I don't see any issues if it gets into Python 3.14.1 (unless I'm missing something).

bedevere-app bot added the awaiting review label Nov 15, 2025

bedevere-app bot mentioned this pull request Nov 15, 2025

getpass's echo_char should not affect keyboard shortcuts #138577

Open

picnixz self-requested a review November 15, 2025 18:08

CuriousLearner requested a review from vstinner November 15, 2025 18:11

picnixz previously requested changes Nov 15, 2025

View reviewed changes

bedevere-app bot added awaiting changes and removed awaiting review labels Nov 15, 2025

Address reviews on dispatcher pattern + handle other ctrl chars

31e35e4

CuriousLearner added awaiting core review and removed awaiting changes labels Nov 16, 2025

bedevere-app bot removed the awaiting core review label Nov 16, 2025

picnixz self-requested a review November 16, 2025 10:26

bedevere-app bot added the awaiting review label Nov 16, 2025

picnixz reviewed Nov 16, 2025

View reviewed changes

Address reviews

b8609bd

CuriousLearner requested a review from picnixz December 6, 2025 22:00

	"""Redraw the entire password line with asterisks."""
	"""Redraw the entire password line with echo_char."""

		self.ctrl = {name: _decode_ctrl_char(value)
		for name, value in ctrl_chars.items()}

Uh oh!

Conversation

CuriousLearner commented Nov 15, 2025 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

picnixz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-app bot commented Nov 15, 2025

Uh oh!

CuriousLearner commented Nov 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CuriousLearner commented Nov 16, 2025

Uh oh!

picnixz commented Nov 16, 2025

Uh oh!

picnixz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CuriousLearner commented Nov 30, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

CuriousLearner commented Nov 15, 2025 •

edited by github-actions bot

Loading

CuriousLearner commented Nov 15, 2025 •

edited

Loading